fix(openapi): the ejector refuses to clobber a file it does not own (#694) - #714
Merged
Conversation
…694) `stitch-openapi --out <dir>` wrote every emitted file unconditionally — `cli.ts:146-147` was a bare loop over `result.files`, then `wrote N file(s)` and exit 0. No existence check, no prompt, no `--force`: grepping the whole package for `force`, `existsSync` or `overwrit` returned nothing, so there was no guard to have missed. The measured rule was "a file it emits it replaces whole, a file it does not emit it ignores", which leaves `--out <the directory you already own>` one flag away from the destructive thing. The reported loss is the point rather than the ergonomics. An owner had added a `drift(Order)` output to the generated tree; a re-run erased it, and afterwards a response missing the `required` `currency` field went from failing to passing silently. The tool's whole claim is that the artefact is source you own, and replacing it at exit 0 with no warning contradicts that. A manifest was already being written and never read back. It is now. The `.stitch-gen.json` gains a `files` list — every path the run wrote, relative to `--out`, including the manifest itself — and the CLI reads the previous run's copy before writing anything. A target that already exists and is NOT named there belongs to the author: the run names every such file, writes nothing at all, and exits 1. A target the manifest does claim is replaced exactly as before, which keeps the ordinary regen path intact. `--force` opts back into the old behaviour, spelled the way `stitch init --force` already is. The existence probe is a new `exists` member on the CLI's IO seam, mirroring core's `CliIO`; `--dry-run` writes nothing and is untouched. The ownership graph the manifest already carried is not quite an ownership LIST: it names operation and schema files but not `client.ts`, `index.ts` or itself, and a flat-layout private schema records `<op>.ts (inlined)`, a marker rather than a path. Hence the explicit list. A manifest written before that list exists is still honoured — derived from the graph plus those three constants — so upgrading does not cost a blanket `--force` to re-adopt a tree this generator really did produce. Also §3 of the same issue, in `planGen`: the manifest recorded the REQUESTED validator tier, so `--validator zod` durably wrote `"validator": "zod"` over a tree containing zero validators. The fallback warning is a build-time stderr line nobody rereads; the manifest is the durable artefact. It records the tier actually emitted now. Refs #694 — §2 (files orphaned by an operation dropped from the spec), §4 (dropped schema keywords) and §5 (the declared error surface) are untouched and stay open, so the issue should not close. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
stitch-openapi --out <dir>overwrote the author's own files with exit 0 and no warning.packages/openapi/src/cli.ts:146-147(on the parent commit) was a bare loop:then
wrote N file(s)on stderr andreturn 0. There was no guard to have missed: grepping all ofpackages/openapi/srcforforce,existsSyncoroverwritreturned zero hits. The measured rulewas a file it emits it replaces whole; a file it does not emit it ignores — so
--out <the directory you already own>is one flag away from the destructive thing.#694 §1 documents the consequence: an owner's
hand-added
drift(Order)output was erased by a regen, after which a response missing therequiredcurrencyfield went from failing to passing silently.An ownership manifest was already written (
gen-openapi.ts:645-648) and never read back.Separately, §3, one line:
gen-openapi.ts:630wrote the requested validator tier into thatmanifest, while
:414-418warns that anything buttypes-onlyfalls back. So--validator zoddurably recorded
"validator": "zod"over a tree with zero validators in it.What changed
.stitch-gen.jsongains afileslist — every path the run wrote, relative to--out,including the manifest itself. The ownership graph it already carried is not quite an ownership
list: it names operation and schema files but not
client.ts,index.tsor itself, and aflat-layout private schema records
<op>.ts (inlined), a marker rather than a path.exists and is not named there belongs to the author: the run names every such file, writes
nothing at all, and exits 1. A target the manifest does claim is replaced exactly as before —
the ordinary regen path is untouched.
--forceopts back into the old behaviour, spelled the waystitch init --forcealready is.--dry-runwrites nothing and is unaffected.fileslist is still honoured, derived from the graph plus thethree always-emitted constants, so upgrading does not cost a blanket
--forceto re-adopt a treethis generator really did produce.
existsmember on the CLI's IO seam, mirroring core'sCliIO.Files touched:
packages/openapi/src/cli.ts,packages/openapi/src/gen-openapi.ts,packages/openapi/test/cli.spec.ts(new),packages/openapi/test/gen-openapi.spec.ts,packages/openapi/README.md,CHANGELOG.md.Tested
New
test/cli.spec.ts(in-memory IO, in the style of core'sstitch initspecs) plus two additionsto
test/gen-openapi.spec.ts— 35 tests pass, 0 fail. Stashing onlypackages/openapi/srcandre-running the suite fails 5 of them, so they pin the fix rather than the status quo.
Covered:
leaves the directory byte-identical (not even the manifest is written);
--forceoverwrites it;--force, including after an owner edit;not named in the error);
filesmanifest still adopts the tree it generated;--dry-runnever trips the guard;covered rather than only the fake standing in for it;
types-onlyin the manifest.Also smoke-tested through the built
bin/stitch-openapi: run into a clean dir →exit 0; re-runover its own tree →
exit 0, regenerated; run into a dir holding a hand-writtenclient.ts→refusing to overwrite 1 file(s) … Nothing was written.,exit 1, file intact; same with--force→
exit 0;--validator zod→"validator": "types-only"in the manifest.Gates
node scripts/check-lint.mjs openapipnpm --filter @stitchapi/openapi check:lintdoes not exist; that package defines nocheck:lintscript (the workspace gate isscripts/check-lint.mjs, filtered here to this package)pnpm --filter @stitchapi/openapi check:typespnpm --filter @stitchapi/openapi testpnpm exec prettier --checkon every changed filenode scripts/check-changelog.mjsnode scripts/check-contract.mjsnode scripts/check-unknown-keys.mjsScope
Refs #694— deliberately notFixes, so the issue stays open. Only §1 and §3 are addressed;§2 (files orphaned by an operation dropped from the spec), §4 (dropped schema keywords) and §5 (the
declared error surface) are untouched.
🤖 Generated with Claude Code